fix: add linter for PR-only changes in samples-go #198
Conversation
Signed-off-by: Saket Maurya <mauryapriyadarshi2004@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR enhances the CI workflow efficiency by dynamically detecting which Go projects have been modified and running the golangci-lint only on those projects, rather than linting all projects in the monorepo regardless of changes.
Changes:
- Added a new
detect-go-projectsjob that identifies modified Go projects by comparing git diffs - Modified the
golangcijob to use a dynamic matrix based on detected changes instead of a static list of all projects - Updated GitHub Actions versions (checkout v3→v4) and enabled Go cache for faster builds
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/golangci-lint.yml
Outdated
| projects=() | ||
| for dir in $dirs; do | ||
| if [ -f "$dir/go.mod" ]; then | ||
| # projects+=("\"$dir\"") |
There was a problem hiding this comment.
Remove commented-out code. This line appears to be leftover from testing different approaches and should be deleted.
| # projects+=("\"$dir\"") |
.github/workflows/golangci-lint.yml
Outdated
| done | ||
|
|
||
| echo "Step 4: build matrix json" | ||
| # matrix=$(printf '%s\n' "${projects[@]}" | jq -R . | jq -cs .) |
There was a problem hiding this comment.
Remove commented-out code. This line appears to be an alternative implementation using jq and should be deleted if it's no longer needed.
| # matrix=$(printf '%s\n' "${projects[@]}" | jq -R . | jq -cs .) |
.github/workflows/golangci-lint.yml
Outdated
| git fetch origin ${{ github.base_ref }} | ||
| diff_base="origin/${{ github.base_ref }}" | ||
| else | ||
| diff_base="HEAD~1" |
There was a problem hiding this comment.
Using HEAD1 for push events may fail on the initial commit or when the repository has shallow clones from other workflows. Consider using a more robust approach, such as comparing against the specific commit SHA from the before field in the push event payload using github.event.before, or handle the case where HEAD1 doesn't exist.
| diff_base="HEAD~1" | |
| # For push events, use the commit SHA from the event payload when available. | |
| if [ "${{ github.event.before }}" != "0000000000000000000000000000000000000000" ]; then | |
| diff_base="${{ github.event.before }}" | |
| else | |
| # Initial commit: diff against an empty tree. | |
| diff_base="$(git hash-object -t tree /dev/null)" | |
| fi |
There was a problem hiding this comment.
fixed this edge case
| run: | | ||
| # Ensure base branch is available locally for diff | ||
| echo "Step 0: prepare diff base" | ||
| if [ "${{ github.event_name }}" = "pull_request" ]; then | ||
| git fetch origin ${{ github.base_ref }} | ||
| diff_base="origin/${{ github.base_ref }}" | ||
| else | ||
| diff_base="HEAD~1" | ||
| fi | ||
|
|
||
| echo "Step 1: get changed files" | ||
| files=$(git diff --name-only "$diff_base"...HEAD) | ||
|
|
||
| # echo "Step 1: get changed files" | ||
| # files=$(git diff --name-only origin/${{ github.base_ref }}...HEAD) | ||
|
|
||
| echo "Step 2: get top-level directories" | ||
| dirs=$(echo "$files" | cut -d/ -f1 | sort -u) | ||
|
|
||
| echo "Step 3: filter go projects" | ||
| projects=() | ||
| for dir in $dirs; do | ||
| if [ -f "$dir/go.mod" ]; then | ||
| # projects+=("\"$dir\"") | ||
| projects+=("$dir") | ||
| fi | ||
| done | ||
|
|
||
| echo "Step 4: build matrix json" | ||
| # matrix=$(printf '%s\n' "${projects[@]}" | jq -R . | jq -cs .) | ||
| if [ ${#projects[@]} -eq 0 ]; then | ||
| matrix="[]" | ||
| else | ||
| joined=$(printf '"%s",' "${projects[@]}") | ||
| joined="[${joined%,}]" | ||
| matrix="$joined" | ||
| fi | ||
|
|
||
| echo "Step 5: final changed go projects :" | ||
| echo "$matrix" | ||
|
|
||
| echo "matrix={\"working-directory\":$matrix}" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
The script does not handle the case where git diff might fail or produce unexpected output. Add error handling with set -e at the beginning of the script to exit on errors, or add explicit error checks after critical commands to ensure the workflow fails gracefully if something goes wrong.
There was a problem hiding this comment.
github workflow by default use the bash -e {0}, so set -e is active
here the doc link - https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#defaultsrunshell
| echo "Step 1: get changed files" | ||
| files=$(git diff --name-only "$diff_base"...HEAD) | ||
|
|
||
| # echo "Step 1: get changed files" | ||
| # files=$(git diff --name-only origin/${{ github.base_ref }}...HEAD) | ||
|
|
||
| echo "Step 2: get top-level directories" | ||
| dirs=$(echo "$files" | cut -d/ -f1 | sort -u) |
There was a problem hiding this comment.
The script does not handle the case where no files have changed. If the files variable is empty, the subsequent commands (cut, sort) will still execute but may produce unexpected results. Consider adding a check after line 46 to handle the empty case explicitly and short-circuit to an empty matrix early.
There was a problem hiding this comment.
I tested this in my fork, and the no-file-change case is already handled, an empty diff results in an empty matrix '[ ]', which correctly prevents the lint job from running, this matches the intended workflow behavior.
Here's the screenshot from my forked version:
.github/workflows/golangci-lint.yml
Outdated
| # echo "Step 1: get changed files" | ||
| # files=$(git diff --name-only origin/${{ github.base_ref }}...HEAD) |
There was a problem hiding this comment.
Remove commented-out code. These lines appear to be leftover from development and should be deleted to keep the workflow clean and maintainable.
|
@SaketV8 pls address the copilot comments |
|
Hi @khareyash05 , I have fixed the issue pointed by the copilot, Kindly review it. Thanks! |
Signed-off-by: Saket Maurya <mauryapriyadarshi2004@gmail.com>
SaketV8
force-pushed
the
fix-pr-lint-workflow
branch
from
482dd8a to
25bd37b
Compare
2 participants
🚀 This PR improves the inefficient Go linting in the CI workflow
✨ The improvements I implemented: